feat: implement BatchLogRecordProcessor environment variables and val…#66
feat: implement BatchLogRecordProcessor environment variables and val…#66harshitt13 wants to merge 4 commits into
Conversation
…idation Signed-off-by: Harshit Kushwaha <find.harshitkushwaha@gmail.com>
Signed-off-by: Harshit Kushwaha <find.harshitkushwaha@gmail.com>
Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
michaelbushe
left a comment
There was a problem hiding this comment.
Nice work, please improve according to the comments. Consistency, simplicity, DRYness and Single Responsibility are always great goals.
Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
robert-northmind
left a comment
There was a problem hiding this comment.
Hey, looks very good. Thanks for the contribution
I just left some comments. Let me know what you think. I am also new to this codebase. so i might not be right in all my thoughts
| /// - 'exportTimeout': Duration for the export timeout | ||
| /// - 'maxQueueSize': int for maximum queue size | ||
| /// - 'maxExportBatchSize': int for maximum export batch size | ||
| static Map<String, dynamic> getBlrpConfig() { |
There was a problem hiding this comment.
One thought here. I know there is an existing pattern here to return Map<String, dynamic> objects. But what if we instead returned like a named dart record. Something like:
typedef BlrpEnvironmentValues = ({
Duration? scheduleDelay,
Duration? exportTimeout,
int? maxQueueSize,
int? maxExportBatchSize,
});Then we get the benefit that we know the types of each entry. and we don't need to use string-keys to find them. I think it would in general make the code a bit safer.
It reduces the need for like as int? or is Duration?
Not sure if we wanna go down this path though.
What are your thoughts on this @michaelbushe ?
| final parsedMaxQueueSize = | ||
| _getPositiveIntEnv(otelBlrpMaxQueueSize, minInclusive: 1); |
There was a problem hiding this comment.
One thought here. Does this pub a bit of Blrp domain knowledge in the OTelEnv parsing? Like that a queue needs to be bigger than 0?
What if the OTelEnv parsing only checks if the env variable exisits and has right type? Like it could warn if an int is a string.
But then the logic to check if the acutal int is bigger than 0, that could happen in the BatchLogRecordProcessorConfig.fromEnvironment()?
| factory BatchLogRecordProcessorConfig.fromEnvironment() { | ||
| final env = OTelEnv.getBlrpConfig(); | ||
|
|
||
| var queueSize = (env['maxQueueSize'] as int?) ?? 2048; |
There was a problem hiding this comment.
We have now this 2048 defined here and in the constructor in line 46 also, and in a few other places in this file.
should we break that our to a private static default variable?
Like: static const _defaultQueueSize = 2048;
| this.exportTimeout = const Duration(seconds: 30), | ||
| }); | ||
|
|
||
| /// Creates a configuration by reading `OTEL_BLRP_*` environment variables |
There was a problem hiding this comment.
What if we make the BatchLogRecordProcessorConfig constructor private, and make all its params required. So that we dont have to provide the defaults there.
And instead, we can only create BatchLogRecordProcessorConfig either via the factory BatchLogRecordProcessorConfig.fromEnvironment() or a factory BatchLogRecordProcessorConfig({...});.
Then in those factories we could do the validation. something like:
factory BatchLogRecordProcessorConfig({
int maxQueueSize = defaultQueueSize,
// ...
}) {
if (!_validQueueSize(maxQueueSize)) {
throw ArgumentError.value(maxQueueSize, 'maxQueueSize');
}
return BatchLogRecordProcessorConfig._(
maxQueueSize: maxQueueSize,
);
}
factory BatchLogRecordProcessorConfig.fromEnvironment() {
final env = OTelEnv.getBlrpConfig();
final queueSize = _validQueueSize(env.maxQueueSize)
? env.maxQueueSize!
: defaultQueueSize;
if (env.maxQueueSize != null && !_validQueueSize(env.maxQueueSize)) {
OTelLog.warn(/* invalid environment value */);
}
return BatchLogRecordProcessorConfig(
maxQueueSize: queueSize,
);
}| /// Returns null when not set, non-numeric, or outside the accepted range. | ||
| /// Warns via [OTelLog.warn] when the raw value is present but unusable | ||
| /// (non-numeric, below [minInclusive], or above [maxInclusive]). | ||
| static int? _getPositiveIntEnv( |
There was a problem hiding this comment.
does this need to be private? if it would be public, we could easily directly test the logic in here. Just a thought
| static BatchLogRecordProcessorConfig buildBatchLogRecordProcessorConfig( | ||
| Map<String, dynamic> blrpConfig, | ||
| ) { |
There was a problem hiding this comment.
I didn't quite follow the need for this one. this is only used for testing right?
is there some other way to structure this. so that we dont need public api for testing only needs?
Closes: #65